feat(retro): add flapping detection to retro analysis skill - #834

1. Protected skills/ file modified 📜 Skill insight § Compliance

This PR modifies skills/retro-analysis/SKILL.md, which is a protected governance/infrastructure
path requiring explicit human review and must not be auto-approved. Ensure appropriate
reviewers/CODEOWNERS sign off before merge.

## Issue description
The PR changes a protected path (`skills/`), which must not be auto-approved and requires explicit human review.

## Issue Context
Protected governance/infrastructure paths require elevated scrutiny. This PR updates the retro-analysis skill content under `skills/`.

## Fix Focus Areas
- skills/retro-analysis/SKILL.md[124-126]

3. Assumed event_payload log line 🐞 Bug ☼ Reliability

The flapping data collector prompt unconditionally claims each dispatch-repo run log contains an
event_payload JSON line with pull_request.head.sha and pull_request.number. Repo documentation
for locating dispatch runs currently relies on timestamp/headBranch correlation and does not
establish this log line as a guaranteed interface, so the new guidance can break flapping detection
if the log format differs or the line is absent.

### Issue description
The new flapping collector instructions depend on a specific `event_payload` JSON log line for correlating runs to PR commits. This is not documented elsewhere as a guaranteed log output, and existing run-finding guidance uses timestamp/headBranch matching.

### Issue Context
`skills/finding-agent-runs/SKILL.md` demonstrates the established approach: find shim runs in the source repo and match dispatch-repo runs by timestamp/headBranch. `scripts/pre-code.sh` and `scripts/pre-fix.sh` mention `event_payload` only as an input-validation concern, not as a logged artifact.

### Fix Focus Areas
- skills/retro-analysis/SKILL.md[124-137]

CRITICAL: Pattern 1 (file oscillation) fires on a single reversal, contradicting the skill's own "When NOT to flag" rule

Pattern 1's definition (line 140: "the same file was changed in two or more consecutive runs, and the changes reverse each other ... lines added in run N were removed in run N+1") triggers on a single N/N+1 reversal — exactly two runs. But the "When NOT to flag" section added in the same diff (lines 155-159) states "A single rework cycle (review requested changes, fix addressed them, review approved) is normal" and "Only flag when you see the same changes being applied and reversed repeatedly." These two sections of the same PR contradict each other: following Pattern 1 literally will flag the default happy-path review-fix cycle (code adds X, review flags X, fix removes X) as flapping, which is precisely the false-positive the exclusion section exists to prevent.

This is compounded by "consecutive runs" being ambiguous given the real workflow sequence is code → review → fix → review → fix, where review runs typically touch no files — if "consecutive" means consecutive workflow runs, oscillation almost never fires; if it means consecutive file-changing runs, it collapses back into the single-reversal false positive. It's also the only one of the three patterns with a 2-run threshold: Pattern 2 requires pass-fail-pass (3 runs) and Pattern 3 requires "more than 2" cycles (3+), so Pattern 1 is inconsistent with its siblings as well as with the exclusion list.

Suggestion: align Pattern 1 with the "repeated reversals" bar used everywhere else in the section — require at least two reversals (e.g. A→B→A across three file-changing runs) rather than a single undo, and explicitly define "consecutive" as consecutive file-changing (code/fix) runs, with review runs used only to correlate finding text rather than for the file-diff comparison.

MEDIUM: <PR_NUMBER> placeholder used in the data-gathering prompt is never derived

The Applicability section (this line) only covers the case where $ORIGINATING_URL is an issue URL ("check whether a PR is linked ... before skipping"); it says nothing about the common case where $ORIGINATING_URL is already a PR URL, and in neither branch does it bind a value to the <PR_NUMBER> placeholder used two lines later in the data-gathering prompt ("Find all code, fix, and review workflow runs related to PR #<PR_NUMBER>", line 136). No earlier section in SKILL.md defines $ORIGINATING_URL, PR_NUMBER, or any variable a subagent could substitute here (the Setup section only defines $REPO_FULL_NAME/$DISPATCH_REPO). A subagent following this literally has no stated source for <PR_NUMBER> in either branch.

Suggestion: add one line per branch — if $ORIGINATING_URL is a PR URL, extract its number directly; if it's an issue URL, use the linked PR's number from closedByPullRequestsReferences — and reference that resolved value explicitly when introducing the data-gathering prompt.

MEDIUM: <REPO> in the data-gathering prompt is not bound to the linked PR's repo in the cross-repo issue case

This PR's own linked issue demonstrates the gap: the issue lives in one repo and the linked PR (this one) lives in a different repo (closedByPullRequestsReferences on the issue resolves to a PR in a different repo than the issue itself). The Applicability check resolves a linked PR from an issue URL but never states which repo <REPO> (used here: "fetch the commit's changed files and CI check-run results from <REPO>") should resolve to when the linked PR lives in a different repo than $REPO_FULL_NAME.

Note this is distinct from the existing bot review comment on this line (which flags that <DISPATCH_REPO>/<REPO> are literal unsubstituted tokens vs. the skill's $DISPATCH_REPO/$REPO_FULL_NAME convention) — even if that comment's suggested fix of substituting $REPO_FULL_NAME is applied, it would resolve to the wrong repo in exactly this cross-repo scenario, since the actual PR/CI data lives in the linked PR's own repo, not the issue's repo.

Suggestion: explicitly bind <REPO> to the repository field returned by the linked PR (from closedByPullRequestsReferences) when flapping detection is entered via the issue-URL branch, noting it may differ from $REPO_FULL_NAME.

MEDIUM: Pattern 2 (test result flipping) needs per-test/file-coverage data the collector never fetches

Pattern 2 requires identifying "a test that passed after run N fails after run N+1, then passes again after run N+2, and the flapping test covers a file the agent modified in the same run" — i.e., both a specific failing test and a test-to-file coverage mapping. The only CI data the collector prompt (line 136) fetches is "CI check-run results," which GitHub's Checks API returns at job/suite granularity (e.g. a single "unit-tests" check), not per-test, and carries no file-coverage mapping. No heuristic is given for how a subagent would derive individual test identity or test-to-file coverage from check-run-level data alone.

Suggestion: either specify a concrete heuristic (e.g. parse per-test names out of CI log output/test-report artifacts, if available) or relax Pattern 2 to what check-run-level data actually supports (e.g. "the same named CI job flips status across 3+ runs while covering the same changed files").

MEDIUM: Hardcoded "more than 2" cycle threshold presents an explicitly unresolved design question as settled

Pattern 3 states as fact that "more than 2 review-fix cycles on the same PR without convergence" is flapping. The design doc this feature implements (docs/problems/flapping-convergence.md in fullsend-ai/fullsend) explicitly says thresholds must vary per repo/task type in its "Thresholds and configuration" section ("A documentation repo might tolerate only 2 review cycles ... A complex backend service might allow 5 cycles ... Default thresholds should be conservative and configurable per repo and per agent role"), and its "Open questions" section lists "What is the right default flapping budget?" as unanswered. This PR hardcodes a fixed, non-configurable "2" in the skill without referencing the doc or noting it as a provisional default.

Suggestion: reference docs/problems/flapping-convergence.md and phrase the threshold as a starting heuristic pending configurable per-repo thresholds, rather than an evidence-based fixed number.

HIGH: Code runs carry no pull_request in event_payload, so PR-number matching silently drops the first file-changing run

The collector prompt tells the subagent to parse pull_request.head.sha / pull_request.number and "confirm the run belongs to this PR". Code runs never carry a pull_request object.

Verified from primary sources in fullsend-ai/fullsend: ADR 0033 states the per-org dispatch.yml "builds a minimal payload from $GITHUB_EVENT_PATH (extracting only issue, pull_request, and comment fields)", and docs/contributing/ci-workflows.md gives the canonical reusable-code.yml concurrency key as fullsend-code-agent-${{ inputs.source_repo }}-${{ fromJSON(inputs.event_payload).issue.number || fromJSON(inputs.event_payload).pull_request.number }} — the platform's own code-agent key falls back to issue.number precisely because pull_request is absent for code dispatches. This repo's skills/finding-agent-runs/SKILL.md independently confirms the trigger: "Code dispatches from issue events when ready-to-code is applied."

The code run is the first file-changing run (it creates the branch and the PR), so under the rule as written it fails confirmation and is discarded, destroying the baseline for Pattern 1's run N / N+1 / N+2 numbering. Conversely, a comment-triggered fix run puts the PR number in issue.number, so issue.number is not always an issue number.

Suggestion: spell out a three-way match rule mirroring the platform's own concurrency key — a run belongs to this PR if pull_request.number == PR_NUMBER, OR issue.number == PR_NUMBER (comment-triggered fix), OR issue.number equals the issue extracted from the agent/{issue}-{slug} branch (code runs). That branch convention is already documented at line 30 of this same file and in skills/finding-agent-runs/SKILL.md.

HIGH: pull_request.head.sha is the pre-run head, so "the commit's changed files" attributes each run the previous run's diff

Lines 159-160 instruct: "For each matched run, fetch the commit's changed files ... from PR_REPO." But the payload is built at dispatch time from the triggering event — ADR 0033 (fullsend-ai/fullsend): the dispatch workflow "builds a minimal payload from $GITHUB_EVENT_PATH" — so head.sha is the branch head before the run does any work.

Every code/fix run is therefore attributed the diff its predecessor produced, shifting the entire sequence by one cycle. That directly corrupts the A→B→A reversal analysis in Pattern 1 (lines 170-174) that this whole section exists to support: the reversal will appear to occur one run earlier than it did, and the last run's actual output is never examined at all.

Useful corollary from the same dispatch model: a review run is triggered by a pull_request event on the pushed commit, so a review run's head.sha is the output commit of the preceding code/fix run — making review runs a reliable per-cycle anchor.

Suggestion: define run N's changes as the diff between head.sha(run N) and head.sha(next run) — equivalently, the head SHA of the review run that follows it — rather than the changed files of head.sha(run N). State this explicitly in the collector prompt, since the naive reading ("the run's commit") is wrong for every code/fix run.

HIGH: Run/PR correlation ignores the source repo, but the dispatch repo is org-wide

$DISPATCH_REPO is ${ORG}/.fullsend (line 19) — a single repo serving every repository in the org. Issue/PR numbers are not unique across repos in an org, so matching on pull_request.number == PR_NUMBER alone can pull a numerically-coincident PR's runs from a completely different repo into the flapping analysis, producing fabricated oscillation.

Disambiguation is available and is already the platform's own practice: source_repo is a first-class dispatch input (internal/harnessdispatch/ref.go: SourceRepo string \json:"source_repo"`; ADR 0034 lists it among the workflow_callinputs), andreusable-code.yml's concurrency key keys on inputs.source_repo` alongside the number for exactly this reason.

Compounding this, DISPATCH_REPO is derived at lines 18-19 from $REPO_FULL_NAME's org, yet lines 144-145 explicitly permit PR_REPO to differ from $REPO_FULL_NAME — when the resolved PR lives under a different org, this line searches the wrong dispatch repo entirely and returns nothing, silently.

Suggestion: require both halves — (a) a run matches only if source_repo (or pull_request.base.repo.full_name) equals PR_REPO AND the number matches; and (b) re-derive DISPATCH_REPO from PR_REPO's org once PR_REPO is resolved, stated alongside the existing "use PR_NUMBER and PR_REPO (not $REPO_FULL_NAME)" instruction at line 147.

HIGH: Data collector cannot supply the line-level diffs Pattern 1 requires

Pattern 1 (lines 170-174) requires detecting that "the same lines in a file are changed, reverted, then changed again" — a repeated A→B→A on specific lines. That needs per-run patches/hunks.

The collector prompt (lines 155-162) only asks for "the commit's changed files" (a file-path list), "the named CI check results", and review comments. It never requests patch content, so an agent following the recipe literally can observe only that the same file path was touched three times. The skill's own "When NOT to flag" list states "Different files changing across runs is normal iteration", and the section repeatedly warns against path-level inference — so the agent will either skip Pattern 1 entirely (no data) or over-flag any file touched three times. Pattern 1 is the flagship pattern of the section and is not executable as specified.

Suggestion: update the collector instructions to fetch per-commit/per-range patches for file-changing runs (the commit or compare diff), and instruct the agent to compare hunks/line ranges across runs rather than file-path lists. This pairs with the head.sha finding above: the diff to fetch is between consecutive runs' head SHAs.

HIGH: Pattern 2 lacks the repeat bar Pattern 1 was given, so it fires on ordinary regress-and-fix

Pattern 2 (lines 175-180) flags whenever the same named CI check goes pass→fail→pass across three or more runs with overlapping changed files. That is exactly the shape of an ordinary single-mistake recovery: code lands green, one fix regresses a test, the next fix restores it. Since "overlapping changed files" is true for nearly every fix sequence on the same PR, this will fire routinely.

The asymmetry is textual and self-contradicting: Pattern 1 was deliberately tightened in this same PR to require a repeated A→B→A and explicitly excludes "a single add-then-remove" as "the normal review-fix cycle", and "When NOT to flag" says "A single rework cycle ... is normal" and "Only flag when the same change is applied and reversed repeatedly". Pattern 2 carries no equivalent safeguard.

The asymmetry is also substantive, not just cosmetic: for line diffs, A→B→A signals a contested change with no stable answer, whereas for CI checks pass is the desired attractor, so pass→fail→pass is convergence, not oscillation. As written, Pattern 2 will file "Flapping detected:" issues against healthy PRs — the exact noise the mandatory "Before proposing: check for existing issues" section exists to prevent.

Suggestion: tighten Pattern 2 to require a second flip (pass→fail→pass→fail, or two full oscillations) before flagging, or require the failure to reappear after an intervening pass on the same check with overlapping agent diffs — mirroring the stricter bar Pattern 1 uses. A single regress-then-recover should be listed under "When NOT to flag".

MEDIUM: "Resolve the linked PR" assumes exactly one linked PR; the only available recipe returns a list of up to 50

The Applicability branch reads as a two-way choice — exactly one linked PR, or none ("If no linked PR exists, skip flapping detection ... Otherwise set PR_NUMBER and PR_REPO from the linked PR").

The actual recipe contradicts that shape: skills/github-forge/SKILL.md returns closedByPullRequestsReferences(first: 50) { nodes { number url author { login } state } } — a list of up to 50, each carrying state. This PR's own originating issue (fullsend-ai/fullsend#5512) resolves to multiple linked PRs across two repos, including the superseded attempt #540. With no tie-break, the retro agent may analyse an abandoned attempt's run history and report flapping for a PR nobody is working on. Note the same query returns no repository field, so PR_REPO can only be derived by parsing each node's url.

Suggestion: state the tie-break explicitly, using the state field the query already returns — prefer the single open linked PR; if none is open, take the most recently updated; if several are open, either skip flapping detection or analyse each separately and say which PR each finding refers to. Also say to derive PR_REPO by parsing the node's url.

MEDIUM: The issue-to-PR linkage reference points at a skill that does not contain the recipe

Lines 141-142 say to "resolve the linked PR using your forge-specific skill's issue-to-PR linkage recipe." Everywhere else in this file "your forge-specific skill" means the forge variant of this skill — see line 13 ("Use the forge-specific retro-analysis skill for CLI recipes"), and lines 43 and 213.

Verified at head: neither skills/retro-analysis/github/SKILL.md nor skills/retro-analysis/gitlab/SKILL.md contains any linkage recipe (the GitHub file covers workflow tracing, log reading, agents-repo discovery, and duplicate search only). The recipe actually lives in the separate shared forge skills — skills/github-forge/SKILL.md has the closedByPullRequestsReferences GraphQL query, and gitlab-forge is the GitLab counterpart. Both are loaded for the retro agent (harness/retro.yaml lists skills/github-forge and skills/gitlab-forge under forge.*.skills), so the capability is present — the pointer just sends the agent to a file that does not have it, and the agent will find nothing at the location the file's own convention names.

Suggestion: name the skill explicitly — "use the github-forge / gitlab-forge skill's linked-PR/MR recipe (closedByPullRequestsReferences / closed_by)" — rather than the ambiguous "your forge-specific skill", which this file uses to mean retro-analysis/{github,gitlab}.

MEDIUM: Run discovery is unbounded, and every existing recipe caps at 10 runs

"Find all code, fix, and review workflow runs related to PR #PR_NUMBER in $DISPATCH_REPO" has no bound and no server-side filter available: dispatch-repo runs are workflow_dispatch with no PR in their ref/branch, so per this section's own premise the only way to identify a run is to download its full log and parse event_payload.

Verified at head, every run-listing recipe the retro agent has uses --limit 10 (skills/retro-analysis/github/SKILL.md: --workflow=code.yml --limit 10, review.yml --limit 10, fix.yml --limit 10; the GitLab file uses per_page=20). In a busy org dispatch repo those windows cover on the order of an hour or two. A PR that flaps for days is exactly the case where the target runs are furthest back, so as written the collector will usually find none of them — or will download hundreds of run logs against the retro harness's 30-minute timeout_minutes (harness/retro.yaml) before synthesis begins.

Suggestion: bound the search explicitly — derive a time window from the PR's createdAt..updatedAt and pass --created <start>..<end> with a raised --limit per workflow, and note that each candidate costs a full log fetch so the window must be bounded before iterating. Reuse the finding-agent-runs discovery recipe rather than reinventing run discovery here.

MEDIUM: "Order runs by commit" is undefined across force-pushes and does not de-duplicate same-SHA reruns

This line says "Order runs by commit and count only file-changing (code/fix) runs as steps". Commit order is only a partial order in practice.

The fix agent rebases and amends, so a head.sha recorded in an earlier run's payload is frequently no longer reachable from the PR head — the rewritten commits are new objects with no ancestry relation to the recorded SHAs, leaving "order by commit" undefined for exactly the long-lived PRs this section targets. Worse, a rebase makes a file look "reverted then re-applied" when nothing oscillated — a direct false positive for Pattern 1.

Separately, workflow reruns on the same SHA produce duplicate runs with no file change, shifting the N / N+1 / N+2 step indexing that both Pattern 1 and Pattern 2 depend on.

Suggestion: order runs by workflow run creation time rather than by commit; collapse same-SHA reruns into a single step; and add force-push/rebase to "When NOT to flag" — if a recorded head.sha is no longer reachable from the PR head, treat the reversal signal as unreliable rather than as oscillation.